Conversation
…fusal Five fixes from the review of apache#69812: (1) OpenAIResponseOperator now calls model_dump(mode='json'), so non-JSON field types (plain enum.Enum, datetime, UUID, ...) come back as their JSON representations instead of live Python objects. The old default (mode='python') would break XCom push for models with a plain enum field, since Airflow's serde only unwraps enums that mix in str/int -- and the API call has already been paid for by then. mode='json' is what airflow.serialization.serde.serializers.pydantic already uses, so this alignment holds the operator's XCom-safe claim for any model. (2) parse_response is now wrapped in try/except ValidationError, re-raising as ValueError. responses.parse() raises pydantic.ValidationError internally when the model's JSON output can't be coerced into text_format -- most commonly when the response is truncated mid-JSON on max_output_tokens. Without the wrap, users got a raw pydantic traceback; now they get a single ValueError shape across all parse failures. (3) The output_parsed=None ValueError now includes the API-reported error and incomplete_details in the message, so users don't need a follow-up OpenAIHook.get_response round-trip to diagnose a refusal. (4) Dropped the docstring claim that structured outputs require 'gpt-4o-2024-08-06 or later' -- the default gpt-4o-mini already supports them (has since its initial release), and specific model claims rot as new models ship. Also removed the contradictory 'use hook directly for previous_response_id chaining' line, since response_kwargs already forwards previous_response_id to the SDK. (5) OpenAIHook.parse_response is now generic (TypeVar bound to BaseModel), returning ParsedResponse[T] instead of ParsedResponse[Any]. Callers get output_parsed typed as T | None, matching the SDK's own signature. Also fixes the failing docs --spellcheck-only CI job by rephrasing 'parseable' -> 'return a structured output' throughout, moves the pydantic import in test_openai.py to module top, adds parse_response to the hook-methods list in openai.rst, drops the redundant model= override from the example DAG, and adds a regression test for both the enum-field dump-mode and the ValidationError-to-ValueError conversion.
|
Thanks @kaxil — this is a genuinely great review. Every one of the five catches
All five landed in
Bonus: the failing Author reviewI read every fix line-by-line before pushing and validated the two behavioral Drafted-by: GitHub Copilot (Claude Opus 4.6); reviewed by @YAshhh29 before posting |
…fusal Five fixes from the review of apache#69812: (1) OpenAIResponseOperator now calls model_dump(mode='json'), so non-JSON field types (plain enum.Enum, datetime, UUID, ...) come back as their JSON representations instead of live Python objects. The old default (mode='python') would break XCom push for models with a plain enum field, since Airflow's serde only unwraps enums that mix in str/int -- and the API call has already been paid for by then. mode='json' is what airflow.serialization.serde.serializers.pydantic already uses, so this alignment holds the operator's XCom-safe claim for any model. (2) parse_response is now wrapped in try/except ValidationError, re-raising as ValueError. responses.parse() raises pydantic.ValidationError internally when the model's JSON output can't be coerced into text_format -- most commonly when the response is truncated mid-JSON on max_output_tokens. Without the wrap, users got a raw pydantic traceback; now they get a single ValueError shape across all parse failures. (3) The output_parsed=None ValueError now includes the API-reported error and incomplete_details in the message, so users don't need a follow-up OpenAIHook.get_response round-trip to diagnose a refusal. (4) Dropped the docstring claim that structured outputs require 'gpt-4o-2024-08-06 or later' -- the default gpt-4o-mini already supports them (has since its initial release), and specific model claims rot as new models ship. Also removed the contradictory 'use hook directly for previous_response_id chaining' line, since response_kwargs already forwards previous_response_id to the SDK. (5) OpenAIHook.parse_response is now generic (TypeVar bound to BaseModel), returning ParsedResponse[T] instead of ParsedResponse[Any]. Callers get output_parsed typed as T | None, matching the SDK's own signature. Also fixes the failing docs --spellcheck-only CI job by rephrasing 'parseable' -> 'return a structured output' throughout, moves the pydantic import in test_openai.py to module top, adds parse_response to the hook-methods list in openai.rst, drops the redundant model= override from the example DAG, and adds a regression test for both the enum-field dump-mode and the ValidationError-to-ValueError conversion.
beb3d9e to
59297e5
Compare
|
Could you also run a sample Dag with the this, and show screenshot of Airflow UI to show status and logs please |
|
Will do. This branch has been developed and unit-tested on a Windows workstation, |
|
Done — ran the Environment: Airflow standalone on this branch ( Sample DAG ( class Person(BaseModel):
name: str
age: int
OpenAIResponseOperator(
task_id="extract_person",
conn_id="openai_default",
model="gpt-4o-mini",
input_text="Extract the name and age from: 'Alice is 30 years old.'",
text_format=Person,
)Result: the task succeeds and pushes the parsed structured output to XCom as a plain dict — {"name": "Alice", "age": 30} — no manual serialization, exactly the flow this PR adds. gpt-4o-mini (the default) handled structured outputs fine, matching your note about dropping the specific-model claim. Screenshots attached below:
|
|
Fixed — dragged the images in this time instead of pasting raw |
|
@YAshhh29 The screenshot doesn't look from the |
…fusal Five fixes from the review of apache#69812: (1) OpenAIResponseOperator now calls model_dump(mode='json'), so non-JSON field types (plain enum.Enum, datetime, UUID, ...) come back as their JSON representations instead of live Python objects. The old default (mode='python') would break XCom push for models with a plain enum field, since Airflow's serde only unwraps enums that mix in str/int -- and the API call has already been paid for by then. mode='json' is what airflow.serialization.serde.serializers.pydantic already uses, so this alignment holds the operator's XCom-safe claim for any model. (2) parse_response is now wrapped in try/except ValidationError, re-raising as ValueError. responses.parse() raises pydantic.ValidationError internally when the model's JSON output can't be coerced into text_format -- most commonly when the response is truncated mid-JSON on max_output_tokens. Without the wrap, users got a raw pydantic traceback; now they get a single ValueError shape across all parse failures. (3) The output_parsed=None ValueError now includes the API-reported error and incomplete_details in the message, so users don't need a follow-up OpenAIHook.get_response round-trip to diagnose a refusal. (4) Dropped the docstring claim that structured outputs require 'gpt-4o-2024-08-06 or later' -- the default gpt-4o-mini already supports them (has since its initial release), and specific model claims rot as new models ship. Also removed the contradictory 'use hook directly for previous_response_id chaining' line, since response_kwargs already forwards previous_response_id to the SDK. (5) OpenAIHook.parse_response is now generic (TypeVar bound to BaseModel), returning ParsedResponse[T] instead of ParsedResponse[Any]. Callers get output_parsed typed as T | None, matching the SDK's own signature. Also fixes the failing docs --spellcheck-only CI job by rephrasing 'parseable' -> 'return a structured output' throughout, moves the pydantic import in test_openai.py to module top, adds parse_response to the hook-methods list in openai.rst, drops the redundant model= override from the example DAG, and adds a regression test for both the enum-field dump-mode and the ValidationError-to-ValueError conversion.
e33ac87 to
a56f704
Compare
| ) from exc | ||
|
|
||
| self.log.info("Generated response %s", parsed.id) | ||
| if parsed.output_parsed is None: |
There was a problem hiding this comment.
One gap left on this path: the SDK parses output regardless of response status, so a status='incomplete' response whose emitted JSON still validates (a truncated list field with no min-length constraint, or content_filter cutting after a complete object) sails through here and pushes partial data to XCom as task success. The window is narrow -- truncation usually breaks the JSON and lands in the ValidationError branch -- but the plain-text path below does check response.status != "completed" while this one doesn't. A one-line status guard before this check, reusing the same details harvesting, closes it; worth a regression test that status='incomplete' with a valid parsed model raises.
| # can't be coerced into ``text_format`` — most commonly because the response | ||
| # was truncated (e.g. ``max_output_tokens`` hit) mid-JSON. Convert to a clean | ||
| # ``ValueError`` so callers see a consistent shape across all parse failures. | ||
| raise ValueError( |
There was a problem hiding this comment.
This branch raises before a ParsedResponse exists, so the message carries no response id and no incomplete_details -- users can't even run hook.get_response(id) to confirm reason='max_output_tokens', and the guide now promises "ValueError with the API-reported status, error and incomplete_details" for this case too. Cheapest fix: name truncation (max_output_tokens) as the likely cause in this message, and split the rst sentence to describe the two branches separately.
|
@kaxil, gentle follow-up on this one. I addressed the review findings, reran the sample Dag against a from-source main build and the real OpenAI Responses API, and posted the Grid, logs, and XCom evidence above. Current checks are green and the PR is mergeable. Could you take another look when convenient, or let me know what still needs work? |
Wraps the SDK's responses.parse(), which turns a Pydantic model into a strict JSON-schema request and returns a ParsedResponse whose output_parsed is an instance of that model, or None when the response carries no parsed output. The method is generic over the model type, mirroring the SDK's own TextFormatT, so callers get output_parsed typed as that model rather than Any.
The response_id and usage pushes move verbatim into _push_response_metadata so the structured-output path added next can record the same metadata. No behaviour change.
Pass a Pydantic BaseModel subclass as text_format and the operator calls OpenAIHook.parse_response instead of create_response, returning the parsed model's model_dump(mode="json") so enums, dates and other non-JSON field types are safe to push to XCom. The structured path fails the task rather than returning partial data. A response whose status is not "completed" raises even when its partial JSON validates, as does one with no parsed output (a refusal, or a tools-only response). The SDK's ValidationError for output it cannot parse, usually truncation at max_output_tokens, is re-raised as ValueError naming the model. Errors carry the response id and whatever the API reported: status, error, incomplete_details, refusal text or output item types. text_format is keyword-only and validated when the operator is constructed, since a pydantic dataclass, which the SDK also accepts, would only fail after the billed call returned. Token ceilings and templated response_kwargs go through _build_response_kwargs() as on the text path, and response_id and usage are pushed before the structured output is checked, so a rejected response still records what it cost.
Responses are real ParsedResponse objects built with model_construct and real output items rather than Mocks, so an SDK field rename breaks the tests instead of production. Covers the JSON-mode dump with a plain Enum (the test fails if mode="json" is reverted), token ceilings reaching parse_response as ints, invalid ceilings failing before any request, XCom metadata including for rejected responses, refusal, tools-only, incomplete-but-valid and failed responses, the ValidationError conversion, and text_format validation.
Adds a structured outputs section with an example Dag task, lists parse_response among the hook's Responses methods, and spells out where the structured path differs from the plain-text one: it fails on an incomplete response instead of returning truncated output, and records response_id and usage before checking the output.
16ff973 to
92cf356
Compare
|
Hi @kaxil, sorry this one went quiet for so long. I pushed fixes for your July 30 review the same day but never replied to the threads, which I should have. I've answered them now. I'm also trying to move and get more used to Linux, so I can run Breeze and the full checks locally before pushing. Main changed this operator quite a bit in the meantime (#72049, #72150, #72151), so instead of just fixing the conflicts I wired structured outputs into those changes:
I tested it on openai 2.37, 2.46 and 2.54, and checked that every safeguard has a test that fails if it's removed. Would you mind taking another look when you get a chance? cc @Lee-W, since this builds on your recent changes to the operator. |
response: Any turned off type checking for every attribute the helper reads. With ParsedResponse[BaseModel], mypy narrows on output.type and content.type and checks each field against the SDK, so a renamed or misspelled field is a type error instead of passing silently.
text_format sat where create_response has model, so parse_response(prompt, "gpt-4o"), written by analogy with create_response, passed the model id as the format. model is now the second positional argument, as in create_response, and text_format is keyword-only, as in the SDK's responses.parse.
With text_format set, a background response comes back queued or in_progress, so the status check raises ValueError while the response keeps running on OpenAI's side. Say so in the response_kwargs docs and in the guide's background note.
|
Thanks for the approval and the extra catches, @kaxil. All three are in, one commit each:
|
kaxil
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier comments. The typing, the parse_response argument order and the background=True note all look good.
One non-blocking edge now that the operator returns a dict: with multiple_outputs=True, a text_format field named usage or response_id gets pushed as its own XCom after execute, overwriting the metadata this operator pushes under those keys. A line in the text_format docs naming them as reserved would cover it.
With multiple_outputs=True, each top-level field of the structured result is pushed as its own XCom after execute returns, so a field named response_id or usage would overwrite the metadata the operator pushes under those keys. The text_format docstring and the guide now say so.











This adds structured outputs to
OpenAIResponseOperator. If you pass a Pydantic model astext_format, the operator callsOpenAIHook.parse_response(a thin wrapper around the SDK'sresponses.parse) and returns the parsed result as a plain dict viamodel_dump(mode="json"), so it's safe to push to XCom. Withouttext_format, nothing changes.When something goes wrong, the task fails instead of passing partial data downstream:
max_output_tokens), even if the partial JSON happens to validateValidationErrorbecomes aValueErrorthat names the modelThe error includes the response id and whatever the API sent back: status, error, incomplete details, the refusal text or the output item types.
It fits the rest of the operator.
max_output_tokens,max_tool_callsandresponse_kwargswork the same for structured requests, andresponse_idandusageare pushed to XCom for both paths. For structured responses they're pushed before the output is checked, so even a rejected response records what it cost.text_formatis keyword-only and checked when the operator is created, because the SDK also accepts pydantic dataclasses, which would only fail after the paid API call.Testing
OpenAIHookand SDK client with only the HTTP layer faked: success, refusal, incomplete-but-valid, truncated JSON, a failed response and the plain-text pathWas generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Claude Opus 5.5) following the guidelines